Fix staticcheck failures for pkg/proxy/... #81886

praseodym · 2019-08-24T15:43:35Z

What type of PR is this?

/kind cleanup
/priority important-longterm
/sig network

What this PR does / why we need it:

Cleanup the pkg/proxy/... issues identified by staticcheck in #81189:

pkg/proxy/healthcheck/proxier_health.go:55:2: field port is unused (U1000)
pkg/proxy/healthcheck/proxier_health.go:162:20: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
pkg/proxy/healthcheck/service_health.go:166:20: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
pkg/proxy/iptables/proxier.go:737:2: this value of args is never used (SA4006)
pkg/proxy/iptables/proxier.go:737:15: this result of append is never used, except maybe in other appends (SA4010)
pkg/proxy/iptables/proxier.go:1287:28: this result of append is never used, except maybe in other appends (SA4010)
pkg/proxy/userspace/proxysocket.go:293:3: this value of n is never used (SA4006)
pkg/proxy/winkernel/metrics.go:74:6: func sinceInMicroseconds is unused (U1000)
pkg/proxy/winkernel/metrics.go:79:6: func sinceInSeconds is unused (U1000)
pkg/proxy/winuserspace/proxier.go:94:2: field portMapMutex is unused (U1000)
pkg/proxy/winuserspace/proxier.go:118:2: field owner is unused (U1000)
pkg/proxy/winuserspace/proxier.go:119:2: field socket is unused (U1000)
pkg/proxy/winuserspace/proxysocket.go:620:4: this value of n is never used (SA4006)

Which issue(s) this PR fixes:

#36858, #81657

Special notes for your reviewer:

I'm not sure about the removal of unused fields in pkg/proxy/winuserspace/proxier.go, hopefully CI will catch any issues that I missed ~~(I couldn't figure out how to do a cross build for only Windows)~~.

Also, this PR overlaps with PR #81563 because that problem was caught by staticcheck as well. I'm happy to rebase once that PR is merged.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-08-24T15:43:43Z

Hi @praseodym. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

praseodym · 2019-08-24T15:47:15Z

/assign @lavalamp

cmluciano · 2019-08-24T17:10:47Z

/ok-to-test

praseodym · 2019-08-24T19:53:49Z

/retest

praseodym · 2019-09-20T15:15:23Z

/retest

vllry · 2019-09-22T04:02:12Z

/assign

pkg/proxy/healthcheck/healthcheck.go

pkg/proxy/winkernel/proxier.go

cmluciano · 2019-09-25T14:41:37Z

pkg/proxy/iptables/proxier.go

 	// Not printing these comments, can reduce size of iptables (in case of large
 	// number of endpoints) even by 40%+. So if total number of endpoint chains
 	// is large enough, we simply drop those comments.
 	if proxier.endpointChainsNumber > endpointChainsNumberThreshold {
-		return
+		return args


The comment above suggests that not returning the args was an explicit decision. I'm not sure that re-adding them is the right choice.

We need return args here, otherwise on L1187 in this file will replace args by nil - see #81886 (comment).

This is fixed in #81563

Should we wait for that PR to merge?

cmluciano · 2019-09-25T14:43:13Z

pkg/proxy/iptables/proxier_test.go

-A KUBE-SVC-3WUAALNGPYZZAWAD -j 
-A  -s 10.0.1.3/32 -j KUBE-MARK-MASQ
-A  -m  -p  -j DNAT --to-destination 10.0.1.3:80
+-A KUBE-SVC-3WUAALNGPYZZAWAD -m comment --comment ns1/svc1: -m statistic --mode random --probability 0.33333 -j 


As mentioned before, it makes sense that they comment sections indeed bloat the output.

The comment in the code there says "So if total number of endpoint chains is large enough, we simply drop those comments" and this small test doesn't exceed the threshold for not adding comments.

The code for adding comments was broken though, as described in #81886 (comment).

cmluciano · 2019-09-25T14:45:15Z

pkg/proxy/userspace/proxysocket.go

@@ -290,7 +290,7 @@ func (udp *udpProxySocket) proxyClient(cliAddr net.Addr, svrConn net.Conn, activ
 			klog.Errorf("SetDeadline failed: %v", err)
 			break
 		}
-		n, err = udp.WriteTo(buffer[0:n], cliAddr)
+		_, err = udp.WriteTo(buffer[0:n], cliAddr)


Why are we ignoring this output? Is the linter complaining that it is not used?

Yes, from the commit message:

pkg/proxy/userspace/proxysocket.go:293:3: this value of n is never used (SA4006)

cmluciano · 2019-09-25T14:47:59Z

pkg/proxy/winuserspace/proxier.go

@@ -99,26 +97,6 @@ type Proxier struct {
 // assert Proxier is a proxy.Provider
 var _ proxy.Provider = &Proxier{}

-// A key for the portMap.  The ip has to be a string because slices can't be map


Are these pieces removed because they are not used?

Yes, from the commit message:

pkg/proxy/winuserspace/proxier.go:88:2: field portMapMutex is unused (U1000) pkg/proxy/winuserspace/proxier.go:112:2: field owner is unused (U1000) pkg/proxy/winuserspace/proxier.go:113:2: field socket is unused (U1000)

(exact line numbers are different due to rebase / other changes)

praseodym · 2019-09-25T18:58:58Z

pkg/proxy/iptables/proxier.go

@@ -1184,7 +1184,7 @@ func (proxier *Proxier) syncProxyRules() {
 				args = append(args[:0],
 					"-A", string(svcChain),
 				)
-				proxier.appendServiceCommentLocked(args, svcNameString)
+				args = proxier.appendServiceCommentLocked(args, svcNameString)


This change means that the return value of appendServiceCommentLocked was previously not used, so comments were never added. PR #81563 was also opened to address this, but because this was also caught by staticcheck I have also fixed it in this PR.

danwinship · 2019-12-12T22:49:20Z

/assign @thockin
/unassign @lavalamp

praseodym · 2019-12-13T21:36:10Z

Rebased and updated to fix some new staticcheck failures (see PR description or commit message for a complete list).

thockin

1 comment for fixup

pkg/proxy/healthcheck/proxier_health.go

thockin · 2019-12-17T00:36:06Z

pkg/proxy/iptables/proxier.go

 	// Not printing these comments, can reduce size of iptables (in case of large
 	// number of endpoints) even by 40%+. So if total number of endpoint chains
 	// is large enough, we simply drop those comments.
 	if proxier.endpointChainsNumber > endpointChainsNumberThreshold {
-		return
+		return args


This is fixed in #81563

Errors from staticcheck: pkg/proxy/healthcheck/proxier_health.go:55:2: field port is unused (U1000) pkg/proxy/healthcheck/proxier_health.go:162:20: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006) pkg/proxy/healthcheck/service_health.go:166:20: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006) pkg/proxy/iptables/proxier.go:737:2: this value of args is never used (SA4006) pkg/proxy/iptables/proxier.go:737:15: this result of append is never used, except maybe in other appends (SA4010) pkg/proxy/iptables/proxier.go:1287:28: this result of append is never used, except maybe in other appends (SA4010) pkg/proxy/userspace/proxysocket.go:293:3: this value of n is never used (SA4006) pkg/proxy/winkernel/metrics.go:74:6: func sinceInMicroseconds is unused (U1000) pkg/proxy/winkernel/metrics.go:79:6: func sinceInSeconds is unused (U1000) pkg/proxy/winuserspace/proxier.go:94:2: field portMapMutex is unused (U1000) pkg/proxy/winuserspace/proxier.go:118:2: field owner is unused (U1000) pkg/proxy/winuserspace/proxier.go:119:2: field socket is unused (U1000) pkg/proxy/winuserspace/proxysocket.go:620:4: this value of n is never used (SA4006)

thockin · 2019-12-23T16:58:08Z

Thanks!

/lgtm
/approve

k8s-ci-robot · 2019-12-23T16:58:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: praseodym, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [thockin]
~~pkg/proxy/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

praseodym · 2019-12-23T20:20:39Z

/retest

thockin · 2019-12-23T21:36:17Z

/retest

thockin · 2019-12-23T22:30:27Z

/retest

k8s-ci-robot added the area/ipvs label Aug 24, 2019

k8s-ci-robot requested review from cmluciano and dnardo August 24, 2019 15:44

k8s-ci-robot assigned lavalamp Aug 24, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 24, 2019

praseodym force-pushed the fix-staticcheck-pkg/proxy branch from cf4fba6 to 65b6a48 Compare August 24, 2019 19:18

praseodym force-pushed the fix-staticcheck-pkg/proxy branch 2 times, most recently from 15112b7 to 0973b8a Compare September 20, 2019 14:30

praseodym force-pushed the fix-staticcheck-pkg/proxy branch from 0973b8a to 3499a27 Compare September 20, 2019 17:13

k8s-ci-robot assigned vllry Sep 22, 2019

vllry reviewed Sep 22, 2019

View reviewed changes

pkg/proxy/healthcheck/healthcheck.go Outdated Show resolved Hide resolved

pkg/proxy/healthcheck/healthcheck.go Outdated Show resolved Hide resolved

johscheuer reviewed Sep 22, 2019

View reviewed changes

pkg/proxy/winkernel/proxier.go Outdated Show resolved Hide resolved

praseodym force-pushed the fix-staticcheck-pkg/proxy branch 2 times, most recently from 2b8a014 to 02bea71 Compare September 22, 2019 12:04

cmluciano suggested changes Sep 25, 2019

View reviewed changes

praseodym commented Sep 25, 2019

View reviewed changes

k8s-ci-robot assigned thockin and unassigned lavalamp Dec 12, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2019

praseodym force-pushed the fix-staticcheck-pkg/proxy branch from 02bea71 to e1a5501 Compare December 13, 2019 21:36

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2019

thockin reviewed Dec 17, 2019

View reviewed changes

praseodym force-pushed the fix-staticcheck-pkg/proxy branch from e1a5501 to a54e5ce Compare December 22, 2019 20:32

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 23, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 23, 2019

thockin mentioned this pull request Dec 23, 2019

Fix handling of service comments in iptables #81563

Closed

k8s-ci-robot merged commit d758fc3 into kubernetes:master Dec 24, 2019

k8s-ci-robot added this to the v1.18 milestone Dec 24, 2019

praseodym deleted the fix-staticcheck-pkg/proxy branch December 24, 2019 06:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix staticcheck failures for pkg/proxy/... #81886

Fix staticcheck failures for pkg/proxy/... #81886

praseodym commented Aug 24, 2019 •

edited

k8s-ci-robot commented Aug 24, 2019

praseodym commented Aug 24, 2019

cmluciano commented Aug 24, 2019

praseodym commented Aug 24, 2019

praseodym commented Sep 20, 2019

vllry commented Sep 22, 2019

cmluciano Sep 25, 2019

praseodym Sep 25, 2019

thockin Dec 17, 2019

praseodym Dec 22, 2019

cmluciano Sep 25, 2019

praseodym Sep 25, 2019

cmluciano Sep 25, 2019

praseodym Sep 25, 2019

cmluciano Sep 25, 2019

praseodym Sep 25, 2019 •

edited

praseodym Sep 25, 2019

danwinship commented Dec 12, 2019

praseodym commented Dec 13, 2019

thockin left a comment

thockin Dec 17, 2019

thockin commented Dec 23, 2019

k8s-ci-robot commented Dec 23, 2019

praseodym commented Dec 23, 2019

thockin commented Dec 23, 2019

thockin commented Dec 23, 2019

Fix staticcheck failures for pkg/proxy/... #81886

Fix staticcheck failures for pkg/proxy/... #81886

Conversation

praseodym commented Aug 24, 2019 • edited

k8s-ci-robot commented Aug 24, 2019

praseodym commented Aug 24, 2019

cmluciano commented Aug 24, 2019

praseodym commented Aug 24, 2019

praseodym commented Sep 20, 2019

vllry commented Sep 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praseodym Sep 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship commented Dec 12, 2019

praseodym commented Dec 13, 2019

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Dec 23, 2019

k8s-ci-robot commented Dec 23, 2019

praseodym commented Dec 23, 2019

thockin commented Dec 23, 2019

thockin commented Dec 23, 2019

praseodym commented Aug 24, 2019 •

edited

praseodym Sep 25, 2019 •

edited